-
Notifications
You must be signed in to change notification settings - Fork 587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(sdk/dotnet): rework on query builder #7208
Conversation
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
But still out of the solution since it under construction. Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
{ | ||
private readonly HttpClient _httpClient; | ||
|
||
public GraphQLClient() : this(Environment.GetEnvironmentVariable("DAGGER_SESSION_PORT")!, Environment.GetEnvironmentVariable("DAGGER_SESSION_TOKEN")!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our first priority is to get it support module. So I think it's ok for now to support only run under dagger run
.
|
||
public class Dagger | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it empty class for now until we make codegen work.
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
Signed-off-by: Thanabodee Charoenpiriyakij <[email protected]>
@wingyplus This is funny coincidence. I just forked and started looking at code generation the other day: https://github.com/MikaelElkiaer/dagger/tree/feat/sdk-dotnet-codegen/sdk/dotnet |
@MikaelElkiaer yeah, I just start exploring the codegen yesterday too. 😃 |
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Would we benefit from using the built in .NET Source Generators? It's Roslyn Powered so can generate code at a compile/build time and can have external files fed into the code generator like json files, config files, query responses etc |
{ | ||
var query = QueryBuilder | ||
.Builder() | ||
.Select("container") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will end up something like the Go SDK?
var container = dagger.Client.Container()
<- QueryBuilder
container.From("image").WithEnv("...").WithExec("...")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the SDK API will construct the graphql query throuh this builder.
|
||
public class QueryBuilderTest | ||
{ | ||
[Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the original was using NUnit, and I've been using XUnit for years. Thoughts on https://devblogs.microsoft.com/dotnet/introducing-mstest-sdk/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pjmagee sorry for late reply. I think it would be great to migrate to it. I add an issue on dagger-dotnet-sdk about this wingyplus/dagger-dotnet-sdk#9
I was also looking into the SDK Docs on how to build an SDK. I can see the most popular GraphQL Client and probably has been for years now is https://chillicream.com/docs/strawberryshake/v13/get-started/console Would this be used in the long run? |
@wingyplus @pjmagee I continued a bit on code generation here: https://github.com/MikaelElkiaer/dagger/tree/feat/sdk-dotnet-codegen/sdk/dotnet But I am considering abanding what I have done so far as I am considering alternatives to C#. |
@MikaelElkiaer that sound interesting. I will take a look when I'm online on my computer. By the way, @pjmagee and I are discuss on codegen tool in Dagger discord in dotnet channel, you can reach us there. |
The reason I would still recommend the Roslyn route because it's now a first class citizen as a source generator for .NET. |
Didnt look to be very friendly with how we would want to interact with dagger graphql. So not looking good unless others also can take a look into strawberry to see if iwe can still benefit from it? Otherwise I am not thinking its suitable anymore from a couple of hours trying to use it with dagger |
@wingyplus Is it worth re-opening with latest changes we have and see if getting this current version merge into Dagger or we wait to make it bit more stable? |
I recommended to opening the PR or discuss with the core team after we make the module supported. And we can iterate things (fix the api, review code, fix bugs) faster than living in the official repository. |
I believe we can close this PR for now and continue discuss on Dagger dotnet discord channel or open an issue at https://github.com/wingyplus/dagger-dotnet-sdk/issues. :) |
In this PR try to modernize DotNet SDK by do the following:
.editorconfig
to makedotnet format
works.QueryBuilder
class and port the logic from Go to it. Mostly feature is covered I guess.DaggerSDK
toDagger.SDK
.DaggerSDKCodegen
out of the solution. We will tackle next after this PR. :)